These features have been unstable for a while, don't look likely to get stabilized soon (unless we e.g drive it...)

rust-lang/rustfmt#4991
rust-lang/rustfmt#3347
rust-lang/rustfmt#5081
rust-lang/rustfmt#5083

What is the implication of removing this? I believe it was added to reduce linking time. If we still want to remove it, the dependencies in both README and Dockerfiles (and Makefile) should be updated.

Do we own linking time? Or is it owned by the developer?

I'd argue that we don't own any cargo config:

• Devs will have their own linkers set up in in ~/.cargo/config.toml. E.g if they've install mold
• Fetching with CLI, cargo target directory etc, that's very developer-owned

The gray area here is --cfg=tokio_unstable, but I think that's a minor feature, and if users want to include it, we should provide instructions.

Note if we supported a devcontainer, we would then own more of the developer experience - linking building etc, but at the moment we're a mess of both (I guess this is why I'm spending so much time here).

We don't really want to be messing with build config too much, because it ends up being pain - we're not build experts, we're rust experts.
One sample is the vectorised instruction support which we've mentioned earlier. This is another excellent example

• chore: custom cargo config to reduce build time #2104 Added this https://github.com/ChainSafe/forest/pull/2104/files#diff-9a4f3e4537ebb7474452d131b0d969d89a51286f4269aac5ef268e712be17268R19
• chore: remove lld from system package deps #2200 Removes the link arg that actually speeds up linking, leaving us with technical debt in our build args 'linker - "clang"' that we're not sure works

BTW this arg does nothing on my machine, you actually need a linker if you want a speedup:

  './do-link.bash -C link-arg=-fuse-ld=mold' ran
    1.02 ± 0.11 times faster than './do-link.bash -C linker=clang -C link-arg=-fuse-ld=mold'
    1.21 ± 0.08 times faster than './do-link.bash -C linker=clang -C link-arg=-fuse-ld=lld'
    1.22 ± 0.08 times faster than './do-link.bash -C link-arg=-fuse-ld=lld'
    4.81 ± 0.32 times faster than './do-link.bash -C linker=clang'
    4.95 ± 0.42 times faster than './do-link.bash'

See benchmarking at 54fb5e8

I'm generally all for build-time tuning, and developer experience, but I think the way we handle it at the moment is a bit haphazard, and we end up dropping balls outside of the actual source code

While the developer is free to tinker with his/her own setup, we do own the CI. The smaller the build times are, the more we can offload to GH runners and not paid buildjet machines which brings real benefits of reduced costs (and more subjective ones as well - we like to have green checkmarks sooner than later).

Thanks for doing the benchmark. I'd love to see mold or lld used in the CI.

All in all, do you agree that we should remove clang from dependencies if it is no longer used anywhere by default?

I second this motion. I'd have more confidence in the correctness of Forest if we used the default configuration.

I mean, I overall agree that using defaults is the way to go. That being said if a couple of lines of configuration that are rarely changed bring 10-20% faster builds for thousands of workflow runs in the future - IMO it's worth having it as long as it is not really exotic piece of technology that changes a lot.

Impact of avx2

Methodology

I built the code with and without avx here: https://github.com/ChainSafe/forest/blob/c5b75126b84b1c5af98057187fe43a3f9a96cdff/build.bash
I ran the benchmark script here https://github.com/ChainSafe/forest/blob/c5b75126b84b1c5af98057187fe43a3f9a96cdff/benchmark.bash

• a warmup run, followed by 20 runs, cleaning the db between each run
• 100 tipsets for validation

I didn't have enough hard drive space to run against mainnet snapshots, so I used calibnet

Importing

Validating

Conclusion

There is no statistically significant difference between avx and non_avx builds.
This makes sense because at no point did I see we were CPU bound - I reckon we're io bound. Maybe if we were memory bound we'd get a speedup, but no user is going to map a ~100GB snapshot into memory, and our code is certainly not there yet.

I have on my side:

Network: mainnet
Snapshot used: 2866560_2023_05_17T06_00_00Z.car
OS: Fedora 37
CPU: Intel Core i3-6300

If you can confirm this on a mainnet snapshot as well, my CPU is pretty old (Skylake), probably worth testing on a recent intel gen.

I would say we can leave those instructions away, and re-assess performance later when needed.

Thanks for assessing this, LGTM.

I find these grouping rules pretty helpful. Any ideas on keeping the functionalities after the change?

It would be nice, but not possible on stable - would be pretty easy to write a custom linter, but probably not a priority right now

(see #2876 (comment))